-
Notifications
You must be signed in to change notification settings - Fork 8
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
WIP @trace
function calls
#366
base: main
Are you sure you want to change the base?
Conversation
…ed values to their corresponding mlir type. These transformed values can be used as keys in a dict (stored in ScopedValue for ease). Cache hits are detected but the cache is not yet used because there is not yet a way to replace the mlir data recursively in a traced object.
lib/ReactantCore/src/ReactantCore.jl
Outdated
@@ -3,6 +3,9 @@ module ReactantCore | |||
using ExpressionExplorer: ExpressionExplorer | |||
using MacroTools: MacroTools | |||
|
|||
using Base.ScopedValues |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ScopedValues
where introduced in Julia 1.11, so how about adding the package? i think Base reimplements it and doesn't just reexport, so maybe put an @static if
based on Julia version?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're a step ahead of me, i just added the same comment myself. Is it fine for ReactantCore to depend on ScopedValues?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
upstream MLIR already depends on ScopedValues and it's a lightweight dependency, so it's fine from my side
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think a static if is needed as the ScopedValues package just reexports Base.ScopedValues
callee=MLIR.IR.FlatSymbolRefAttribute(f_name), | ||
) | ||
|
||
seen_results = Reactant.OrderedIdDict() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I first copy a traced structure using make_tracer
, TracedSetPath
. This structure can I then fill in by going by mutating the objects in seen
. This seems to work well but maybe someone has a better approach.
@@ -287,12 +289,16 @@ function compile_mlir(f, args; kwargs...) | |||
end | |||
end | |||
|
|||
const callcache = ScopedValue{Dict}() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems like a simple way to store the cache. Fancier solutions (i.e. integrating with absint?) are probably possible as well but not sure whether it's worth it to investigate as of now.
@@ -3,6 +3,9 @@ module ReactantCore | |||
using ExpressionExplorer: ExpressionExplorer | |||
using MacroTools: MacroTools | |||
|
|||
using Base.ScopedValues | |||
const enable_tracing = ScopedValue{Bool}(false) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we simply want to trace function calls everytime in make_mlir_fn
. I added this scopedvalue that is set in the beginning so it's just a simple check.
If it's okay to use ScopedValues in ReactantCore, ScopedValues probably needs to be added to project.toml for backwards compatibility (?)
no caching, multi-return, kwargs, ... yet.